Skip to content

feat(odsp-client): Add OdspContainerServices readonly and sensitivity label event emitters #25800

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat(odsp-client): Add OdspContainerServices readonly and sensitivity label event emitters #25800

wants to merge 7 commits into from
+116 −8

Conversation

@znewton
Copy link
Contributor

@znewton znewton commented Nov 3, 2025

Description

Portion of #25597 to add read-only state and sensitivity labels events to OdspContainerServices.

Reviewer Guidance

I think this PR could be better if it separated read-only events and sensitivity label events, but it will introduce either merge conflicts or blocking on either. I think it would be weird to extend IEventProvider without any events, which would allow for async feature introduction.

@github-actions github-actions bot added dependencies Pull requests that update a dependency file base: main PRs targeted against main branch labels Nov 3, 2025
@github-actions github-actions bot added the public api change Changes to a public API label Nov 4, 2025
Josmithr
Josmithr reviewed Nov 10, 2025
View reviewed changes
ChumpChief
ChumpChief reviewed Nov 11, 2025
View reviewed changes
packages/service-clients/odsp-client/src/interfaces.ts
* Gets the read-only state of the container, if available.
* @returns The read-only state (true when readonly, false when editable), or undefined if not available.
*/
getReadOnlyState(): boolean | undefined;
Copy link
Contributor

@ChumpChief ChumpChief Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general readonly state is not service-specific, so I'd probably expect to find it on IFluidContainer rather than a container services interface.

Copy link
Contributor Author

@znewton znewton Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a handy way of exposing functionality via ODSP's beta API, rather than increasing scope to FluidContainer's public API. If you feel this needs to be added to the public API of FluidContainer instead, then we can increase the scope to do that. I worry it will prolong the timeline though, given the public IFluidContainer is a pretty tight subset of all container properties and events

Copy link
Contributor

@Josmithr Josmithr Nov 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe let's leave a @privateRemarks block comment noting that this should probably move to the base interface. I agree that moving it probably makes sense, but I don't think we need to block things on that.

Copy link
Contributor Author

@znewton znewton Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, ya know what, looking at this again, I can get the same effect by doing what I did in other PRs via OdspFluidContainer

@znewton znewton marked this pull request as ready for review November 11, 2025 18:57
@znewton znewton requested a review from a team as a code owner November 11, 2025 18:57
Copilot AI review requested due to automatic review settings November 11, 2025 18:57
Copilot finished reviewing on behalf of znewton November 11, 2025 18:59
Copilot AI reviewed Nov 11, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds event emitters for read-only state and sensitivity label changes to OdspContainerServices, enabling consumers to listen for container state changes. The implementation makes OdspContainerServices extend TypedEventEmitter and implement IDisposable for proper resource management.

Key changes:

  • Introduces IOdspContainerServicesEvents interface with readOnlyStateChanged and sensitivityLabelChanged events
  • Implements event forwarding from underlying IContainer events to service-level events
  • Adds getReadOnlyState() method to query current read-only state
  • Adds disposal pattern to properly clean up event listeners

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/service-clients/odsp-client/src/interfaces.ts Adds IOdspContainerServicesEvents interface and extends OdspContainerServices with IEventProvider and IDisposable
packages/service-clients/odsp-client/src/odspContainerServices.ts Implements event emitters, disposal pattern, and getReadOnlyState() method
packages/service-clients/odsp-client/src/index.ts Exports new IOdspContainerServicesEvents interface
packages/service-clients/odsp-client/package.json Adds dependency on @fluid-internal/client-utils for TypedEventEmitter
pnpm-lock.yaml Updates lockfile with new dependency
PACKAGES.md Updates layer dependencies to include Client-Utils
packages/service-clients/odsp-client/api-report/*.api.md Updates API surface documentation
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

@github-actions
Copy link
Contributor

github-actions bot commented Nov 11, 2025

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> [email protected] serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  249793 links
    1772 destination URLs
    2010 URLs ignored
       0 warnings
       0 errors

Josmithr
Josmithr reviewed Nov 13, 2025
View reviewed changes
packages/service-clients/odsp-client/src/interfaces.ts
/**
* Provides an object that facilitates obtaining information about users present in the Fluid session, as well as listeners for roster changes triggered by users joining or leaving the session.
*/
audience: IOdspAudience;

/**
* Gets the read-only state of the container, if available.
Copy link
Contributor

@Josmithr Josmithr Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough docs! These are great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChumpChief ChumpChief ChumpChief left review comments

Copilot code review Copilot Copilot left review comments

@Josmithr Josmithr Josmithr approved these changes

Assignees

No one assigned

Labels

base: main PRs targeted against main branch dependencies Pull requests that update a dependency file public api change Changes to a public API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@znewton @ChumpChief @Josmithr